Skip to content

Restore netplan config even when exception is raised (bugfix)#1777

Merged
pieqq merged 6 commits intomainfrom
fix-1674-restore-netplan-config
Mar 27, 2025
Merged

Restore netplan config even when exception is raised (bugfix)#1777
pieqq merged 6 commits intomainfrom
fix-1674-restore-netplan-config

Conversation

@pieqq
Copy link
Copy Markdown
Collaborator

@pieqq pieqq commented Mar 6, 2025 •
edited
Loading

Description

Use context managers to handle original netplan config backup and restore, as well as the test config writing and removing.

Modify function netplan_apply_config to raise SystemExit if the
subprocess call to netplan failed for any reason.

  • journal entries are still printed out to the log if there is any
    SystemExit
  • test config removal and original config restoration are run regardless
    of what happens

Resolved issues

Fix #1674

https://warthogs.atlassian.net/browse/CHECKBOX-1718

Documentation

Tests

unit tests updated

pieqq added 2 commits March 6, 2025 23:30
@pieqq
Delete test config and restore original config even when an exception…
0253a5e 
… is raised

Modify function `netplan_apply_config` to raise SystemExit if the
subprocess call to netplan failed for any reason.

By doing this, the `main` function can then use the
`try...except...finally` clause, which ensures:

- journal entries are still printed out to the log if there is any
SystemExit
- test config removal and original config restoration are ran regardless
of what happens

A `try...finally` clause is also used when applying the netplan config
after restoring the original files to make sure journal entries are
output to log regardless of the outcome.
@pieqq
netplan apply function does not return boolean anymore
7b0b989 
Fix #1674
@pieqq pieqq requested a review from hanhsuan March 6, 2025 22:34
@pieqq pieqq changed the title Restore netplan config even when exception is raised Restore netplan config even when exception is raised (bugfix) Mar 6, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 6, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 90.24390% with 4 lines in your changes missing coverage. Please review.

Project coverage is 49.98%. Comparing base (7817ae2) to head (def1933).
Report is 137 commits behind head on main.

Files with missing lines Patch % Lines
providers/base/bin/wifi_client_test_netplan.py 90.24% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1777      +/-   ##
==========================================
+ Coverage   49.63%   49.98%   +0.34%     
==========================================
  Files         377      378       +1     
  Lines       40630    40786     +156     
  Branches     6830     6860      +30     
==========================================
+ Hits        20168    20388     +220     
+ Misses      19740    19672      -68     
- Partials      722      726       +4
Flag Coverage Δ
provider-base 26.34% <87.80%> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hanhsuan
Copy link
Copy Markdown
Contributor

hanhsuan commented Mar 7, 2025

Tested on 202412-36142, the config of netplan is deleted if there is no checkbox.conf.

@pieqq
Copy link
Copy Markdown
Collaborator Author

pieqq commented Mar 7, 2025

After adding the fix in commit 9711a32, the execution seems to run correctly, that is, if SSID is omitted, the test will fail with an error message ("A SSID is required for the test"), but the config will be restored.

Hook25
Hook25 requested changes Mar 11, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look very good to me ngl. Catching BaseExceptions like SystemExit is in general not great. My best solution if I had to choose is to make the config a context manager, that would be super gorgeous

with generate_test_config(...) as config_data:
    ...

If you really don't want to do that, you can always use atexit, but I would advise you against that, it is not super clean as it introduces a global side effect to closing the application which is always a bit yucky if you can avoid it

@pieqq pieqq marked this pull request as draft March 26, 2025 15:30
@pieqq
Copy link
Copy Markdown
Collaborator Author

pieqq commented Mar 26, 2025 •
edited
Loading

I tested my latest changes (90eebef) on a device, and the tests fail without an SSID, but the config files are restored:

==============[ Running job 1 / 1. Estimated time left: 0:00:15 ]===============
----[ Connect to unencrypted 802.11be Wi-Fi network on wlp0s20f3 - netplan ]----
ID: com.canonical.certification::wireless/wireless_connection_open_be_np_wlp0s20f3
Category: com.canonical.plainbox::wireless
... 8< -------------------------------------------------------------------------
Interface wlp0s20f3 using module iwlwifi
(...)
## Backup any existing netplan configuration files
Clear backup location
Backing up from /etc/netplan
  /etc/netplan/90-NM-21dbe869-a81c-481b-9c30-5dd25bda5ad7.yaml
Backing up from /etc/netplan
  /etc/netplan/01-network-manager-all.yaml
Backing up from /lib/netplan
  /lib/netplan/00-network-manager-all.yaml

## Delete any existing netplan configuration files
  /etc/netplan/90-NM-21dbe869-a81c-481b-9c30-5dd25bda5ad7.yaml
  /etc/netplan/01-network-manager-all.yaml
  /lib/netplan/00-network-manager-all.yaml

## Generate a test netplan configuration
## Restore configuration files
Restoring:
  /etc/netplan/90-NM-21dbe869-a81c-481b-9c30-5dd25bda5ad7.yaml
  /etc/netplan/01-network-manager-all.yaml
  /lib/netplan/00-network-manager-all.yaml
+ netplan --debug apply

A SSID is required for the test
------------------------------------------------------------------------- >8 ---
Outcome: job failed

$ ll /*/netplan/
/etc/netplan/:
total 24
drwxr-xr-x   2 root root  4096 Mar 26 17:10 ./
drwxr-xr-x 139 root root 12288 Mar 26 16:58 ../
-rw-r--r--   1 root root   104 Mar 26 17:10 01-network-manager-all.yaml
-rw-------   1 root root   727 Mar 26 17:10 90-NM-21dbe869-a81c-481b-9c30-5dd25bda5ad7.yaml

/lib/netplan/:
total 16
drwxr-xr-x  2 root root 4096 Mar 26 17:10 ./
drwxr-xr-x 97 root root 4096 Mar 26 16:58 ../
-rw-------  1 root root  104 Mar 26 17:10 00-network-manager-all.yaml
-rw-r--r--  1 root root  204 Aug 14  2024 PLACEHOLDER

@pieqq pieqq marked this pull request as ready for review March 27, 2025 11:20
Hook25
Hook25 approved these changes Mar 27, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM

@pieqq pieqq merged commit 47b58dd into main Mar 27, 2025
22 checks passed
@pieqq pieqq deleted the fix-1674-restore-netplan-config branch March 27, 2025 14:46
stanley31huang pushed a commit that referenced this pull request Mar 28, 2025
@pieqq @stanley31huang
Restore netplan config even when exception is raised (bugfix) (#1777)
199f4a5 
* Delete test config and restore original config even when an exception is raised

Modify function `netplan_apply_config` to raise SystemExit if the
subprocess call to netplan failed for any reason.

By doing this, the `main` function can then use the
`try...except...finally` clause, which ensures:

- journal entries are still printed out to the log if there is any
SystemExit
- test config removal and original config restoration are ran regardless
of what happens

A `try...finally` clause is also used when applying the netplan config
after restoring the original files to make sure journal entries are
output to log regardless of the outcome.

* netplan apply function does not return boolean anymore

Fix #1674

* Catch FileNotFoundError when netplan test config absent

* Use context managers to handle config backup/restoration

* Make sure test config is deleted

* Update unit tests
LiaoU3 pushed a commit that referenced this pull request Apr 14, 2025
@pieqq
Restore netplan config even when exception is raised (bugfix) (#1777)
86d6a59 
* Delete test config and restore original config even when an exception is raised

Modify function `netplan_apply_config` to raise SystemExit if the
subprocess call to netplan failed for any reason.

By doing this, the `main` function can then use the
`try...except...finally` clause, which ensures:

- journal entries are still printed out to the log if there is any
SystemExit
- test config removal and original config restoration are ran regardless
of what happens

A `try...finally` clause is also used when applying the netplan config
after restoring the original files to make sure journal entries are
output to log regardless of the outcome.

* netplan apply function does not return boolean anymore

Fix #1674

* Catch FileNotFoundError when netplan test config absent

* Use context managers to handle config backup/restoration

* Make sure test config is deleted

* Update unit tests
mreed8855 pushed a commit that referenced this pull request Jul 30, 2025
@pieqq @mreed8855
Restore netplan config even when exception is raised (bugfix) (#1777)
14b0a92 
* Delete test config and restore original config even when an exception is raised

Modify function `netplan_apply_config` to raise SystemExit if the
subprocess call to netplan failed for any reason.

By doing this, the `main` function can then use the
`try...except...finally` clause, which ensures:

- journal entries are still printed out to the log if there is any
SystemExit
- test config removal and original config restoration are ran regardless
of what happens

A `try...finally` clause is also used when applying the netplan config
after restoring the original files to make sure journal entries are
output to log regardless of the outcome.

* netplan apply function does not return boolean anymore

Fix #1674

* Catch FileNotFoundError when netplan test config absent

* Use context managers to handle config backup/restoration

* Make sure test config is deleted

* Update unit tests
mreed8855 pushed a commit that referenced this pull request Jul 31, 2025
@pieqq @mreed8855
Restore netplan config even when exception is raised (bugfix) (#1777)
fb6d498 
* Delete test config and restore original config even when an exception is raised

Modify function `netplan_apply_config` to raise SystemExit if the
subprocess call to netplan failed for any reason.

By doing this, the `main` function can then use the
`try...except...finally` clause, which ensures:

- journal entries are still printed out to the log if there is any
SystemExit
- test config removal and original config restoration are ran regardless
of what happens

A `try...finally` clause is also used when applying the netplan config
after restoring the original files to make sure journal entries are
output to log regardless of the outcome.

* netplan apply function does not return boolean anymore

Fix #1674

* Catch FileNotFoundError when netplan test config absent

* Use context managers to handle config backup/restoration

* Make sure test config is deleted

* Update unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Hook25 Hook25 Hook25 approved these changes

@hanhsuan hanhsuan Awaiting requested review from hanhsuan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

if generate_test_config failed, wifi_client_test_netplan.py won't restore config file

3 participants

@pieqq @hanhsuan @Hook25